Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Remove panic on corruped storage#3997

Merged
bkchr merged 1 commit intomasterfrom
gui-fix-storage-decode-error
Nov 22, 2019
Merged

Remove panic on corruped storage#3997
bkchr merged 1 commit intomasterfrom
gui-fix-storage-decode-error

Conversation

@gui1117
Copy link
Contributor

@gui1117 gui1117 commented Nov 1, 2019

Fix corrupted storage panic

In the long term we should implement #3700

@gui1117 gui1117 requested a review from kianenigma as a code owner November 1, 2019 15:32
@gui1117 gui1117 added the A0-please_review Pull request needs code review. label Nov 1, 2019
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to call into the else branch below to write the correct head.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I was mixed about what was the best and conclude the least we touch things the best it is.

Here either:

  • we let the head where it is, knowing that the list has been cut in two, with second part having a wrong head.
  • move the head to next values potentially cutting a large part of the list.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But this means we will print out this message over and over again. (Given there is no modifcation in between)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this code should attempt to fix stuff, but ok maybe we can do it.

Then we should also maybe:

  • remove entry when in storage::get decode has failing,
  • set the next to None when enumeration is broken,

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh ya ok at least we can make previous being None for the next element. indeed

@gui1117 gui1117 added the A3-in_progress Pull request is in progress. No review needed at this stage. label Nov 1, 2019
@bkchr
Copy link
Member

bkchr commented Nov 11, 2019

Any update?

@gui1117
Copy link
Contributor Author

gui1117 commented Nov 11, 2019

actually I'm confused about what to do:
for remove_linkage it seems we could do:

  • if both previous key and next key are corrupted (like the linkage was the actual value corrupted that somehow decoded correctly) then do no touch anything
  • else if previous key is corrupted then change head and set the previous of the next one to None.
  • else if next key is corrupted then set None to the next of the previous one.

Is that correct ? I still kind of prefer not touching anything if possible but if you agree on this then I make it.

@gavofyork
Copy link
Member

looks strictly better than before, which is enough for a merge.

@gavofyork gavofyork added A3-needsresolving and removed A0-please_review Pull request needs code review. A3-in_progress Pull request is in progress. No review needed at this stage. labels Nov 22, 2019
@gavofyork gavofyork added this to the polkadot-0.6.18 milestone Nov 22, 2019
@gui1117 gui1117 force-pushed the gui-fix-storage-decode-error branch from 2c7b473 to f5304de Compare November 22, 2019 13:42
@gui1117
Copy link
Contributor Author

gui1117 commented Nov 22, 2019

Resolved

@gui1117 gui1117 added A0-please_review Pull request needs code review. and removed A3-needsresolving labels Nov 22, 2019
@gavofyork gavofyork removed the A0-please_review Pull request needs code review. label Nov 22, 2019
@gavofyork gavofyork requested a review from bkchr November 22, 2019 15:16
@bkchr bkchr merged commit 1398042 into master Nov 22, 2019
@bkchr bkchr deleted the gui-fix-storage-decode-error branch November 22, 2019 15:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

Comments